Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PICARD-2751: Avoid deprecated importlib APIs, full Python 3.12 compatibility #2307

Merged
merged 11 commits into from
Sep 11, 2023

Conversation

phw
Copy link
Member

@phw phw commented Sep 9, 2023

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

Picard's plugin loading system must be updated to avoid long deprecated and inf Python 3.12 finally removed APIs of the importlib and zipimport modules.

This PR replaces #2134

Solution

At the core the change is to use find_spec in combination with exec_module to load the modules. A large benefit is that both the normal module loading via importlib as well as the zipimporter can now use the exactly same API.

Unfortunately for zipimporter we need to maintain the legacy loading code with find_module + load_module for compatibility with Python < 3.10.

This PR maintains full compatibility with the existing plugin systems. Overall there is likely a lot that we could further refactor and trim down, but that is something to be reserved for Picard 3.0.

To properly test the result this PR also enables CI testing with Python 3.12.0rc1

test/test_plugins.py Fixed Show fixed Hide fixed
test/test_plugins.py Fixed Show fixed Hide fixed
@phw phw force-pushed the python-3.12-compat branch from 0ea9a64 to 0e65171 Compare September 9, 2023 14:42
test/test_plugins.py Fixed Show fixed Hide fixed
test/test_plugins.py Fixed Show fixed Hide fixed
test/test_plugins.py Fixed Show fixed Hide fixed
test/test_plugins.py Fixed Show fixed Hide fixed
@phw phw force-pushed the python-3.12-compat branch from 0e65171 to 1944a0e Compare September 9, 2023 14:55
@hboetes
Copy link

hboetes commented Sep 9, 2023

FYI I just tried your PR, and it works, but I get these errors.

E: 17:15:51,086 config.__init__:424: Option setting/rsgain_command already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:353: in ReplayGain2OptionsPage
        TextOption("setting", "rsgain_command", "rsgain"),
E: 17:15:51,088 config.__init__:424: Option setting/album_tags already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:354: in ReplayGain2OptionsPage
        BoolOption("setting", "album_tags", True),
E: 17:15:51,090 config.__init__:424: Option setting/true_peak already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:355: in ReplayGain2OptionsPage
        BoolOption("setting", "true_peak", False),
E: 17:15:51,091 config.__init__:424: Option setting/reference_loudness already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:356: in ReplayGain2OptionsPage
        BoolOption("setting", "reference_loudness", False),
E: 17:15:51,093 config.__init__:424: Option setting/target_loudness already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:357: in ReplayGain2OptionsPage
        IntOption("setting", "target_loudness", -18),
E: 17:15:51,094 config.__init__:424: Option setting/clip_mode already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:358: in ReplayGain2OptionsPage
        IntOption("setting", "clip_mode", CLIP_MODE_POSITIVE),
E: 17:15:51,096 config.__init__:424: Option setting/max_peak already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:359: in ReplayGain2OptionsPage
        IntOption("setting", "max_peak", 0),
E: 17:15:51,097 config.__init__:424: Option setting/opus_mode already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:360: in ReplayGain2OptionsPage
        IntOption("setting", "opus_mode", OPUS_MODE_STANDARD),
E: 17:15:51,099 config.__init__:424: Option setting/opus_m23 already declared
at /home/han/.config/MusicBrainz/Picard/plugins/replaygain2.zip/replaygain2/__init__.py:361: in ReplayGain2OptionsPage
        BoolOption("setting", "opus_m23", False)
Time: 0h:00m:06s                                                                                                                                                              

@hboetes
Copy link

hboetes commented Sep 9, 2023

And it crashes upon changing the configuration.

@phw
Copy link
Member Author

phw commented Sep 9, 2023

@hboetes Thanks for the feedback. The log output is quite interesting. I debugged this a bit. The plugin code gets run twice at exactly the line https://github.com/metabrainz/picard/pull/2307/files#diff-c2f162faf440349fc5556185e9cd8b4b3cf0f35235eb561eb3fd7261651a2fbfR356 . That means the spec.loader.exec_module(plugin_module) call is enough for this to happen. Unsure how to avoid this.

And it crashes upon changing the configuration.

Can you elaborate? Any error output on the terminal?

@hboetes
Copy link

hboetes commented Sep 9, 2023

Yes, this actually happened after pressing the settings button, the first time it complains, the second time it crashes.

Edit: That's a bug in the replaygain plugin, after disabling that by hacking in the config that problem is gone.

Traceback (most recent call last):
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/mainwindow.py", line 1313, in show_options
    options_dialog = OptionsDialog.show_instance(page, self)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 156, in show_instance
    instance = cls.get_instance(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 150, in get_instance
    cls._instance = cls(*args, **kwargs)
                    ^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 168, in __init__
    self.add_pages(None, default_page, self.ui.pages_tree)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 113, in add_pages
    self.add_pages(page.NAME, default_page, item)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 104, in add_pages
    for foo, bar, page in sorted(pages):
                          ^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'ReplayGain2OptionsPage' and 'ReplayGain2OptionsPage'
Traceback (most recent call last):
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/mainwindow.py", line 1313, in show_options
    options_dialog = OptionsDialog.show_instance(page, self)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 156, in show_instance
    instance = cls.get_instance(*args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/__init__.py", line 150, in get_instance
    cls._instance = cls(*args, **kwargs)
                    ^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 168, in __init__
    self.add_pages(None, default_page, self.ui.pages_tree)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 113, in add_pages
    self.add_pages(page.NAME, default_page, item)
  File "/usr/local/lib64/python3.12/site-packages/picard/ui/options/dialog.py", line 104, in add_pages
    for foo, bar, page in sorted(pages):
                          ^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'ReplayGain2OptionsPage' and 'ReplayGain2OptionsPage'
[1]    2000396 IOT instruction  picard

@hboetes
Copy link

hboetes commented Sep 9, 2023

For the rest it seems to be working. 👍

Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me, I tested and didn't find any issue, good job!

picard/pluginmanager.py Outdated Show resolved Hide resolved
@phw phw force-pushed the python-3.12-compat branch from 0e98162 to a7c9504 Compare September 10, 2023 11:11
@phw
Copy link
Member Author

phw commented Sep 10, 2023

I have fixed the double execution in 4db506c .

Essentially this happened for all plugins that themselves have relative imports. When running spec.loader.exec_module the module is not yet sys.modules. So if the module does a relative import on itself the module gets imported again. This change feels kind of like a hack, but also the existing behavior using the APIs seems slightly off. I'm not fully sure, but the solution / workaround works.

In the last commit I also changed a bit how plugins get loaded if they exist in multiple directories. The original implementation first tried to load from system plugin dir, but if it also existed in the user plugin dir it "unloaded" the previous plugin and loaded the new one. Now because running exec_module causes the module body to run this could cause double execution again. You cannot undo having run the code, and running it twice can have side effects. So now it first tries user dir and later on ignores the same plugin in other directories again.

Overall it just shows again that we need to separate plugin metadata from code for the next plugin system.

Overall I'm quite happy with how well this works now. This really should in practice work as before but using the new APIs.

Also it shows that importlib provides really good interface now for us to implement plugin loading and if done properly we can for the next iteration of the plugin system simplify the loading a lot.

picard/pluginmanager.py Fixed Show fixed Hide fixed
@phw phw force-pushed the python-3.12-compat branch from d9925c0 to 6d9528c Compare September 10, 2023 11:33
@phw phw requested a review from zas September 10, 2023 11:45
phw added 2 commits September 10, 2023 13:47
If a plugin is already loaded avoid running its code again. This avoids
double execution in case the plugin is in both the user and system plugin
directory.
@phw phw force-pushed the python-3.12-compat branch from 6d9528c to 83ea460 Compare September 10, 2023 11:47
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good, just 2 minor comments

picard/pluginmanager.py Outdated Show resolved Hide resolved
picard/pluginmanager.py Outdated Show resolved Hide resolved
@phw phw force-pushed the python-3.12-compat branch from 702a769 to 7d84330 Compare September 10, 2023 14:40
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@phw phw merged commit b1c2874 into metabrainz:master Sep 11, 2023
@phw phw deleted the python-3.12-compat branch September 11, 2023 06:30
@hboetes
Copy link

hboetes commented Sep 11, 2023

Thanks for the speedy fix. One note, there are various plugins that won't work with python-3.12, like the replaygain plugin. Perhaps a heads-up to the plugin developers is warranted.

@phw
Copy link
Member Author

phw commented Sep 12, 2023

@hboetes the replaygain plugin is actually supposed to work after the latest changes from this PR. At least it does so for me. Do you have any different issue now?

If there is compatibility issue with one of the existing plugins we should of course fix it, but I'm currently not aware of one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants